-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
console.lua: update the select layout #15711
Conversation
33afa69
to
7bd0fda
Compare
Download the artifacts for this pull request: |
How do you ensure that the "tall characters" are at most 10% larger than normal characters, but not more? |
You don't, it's a heuristic. It works with the fonts I tried and can be increased if it doesn't work with some others. |
Well it was kasper's request to do it like this. I don't use background-box personally. |
LGTM |
7bd0fda
to
dfe0518
Compare
It's is improvement over current status, feel free to come up with better ideas for this problem. @guidocella said that |
It has appearance regression and not 100% improvement.
Here is a bounding box that doesn't require compute_bounds: You already know the total height of the box because you know the number of lines. The width can simply be the whole window width. |
It has appearance regression and not 100% improvement. See, I find separate buttons with separate background just as nice. We could compute bounds of longest string in selections, but it still means that we need to draw custom background and disable the style of default background, but I guess this can be done, wit tags for the events. |
This looks fine to me too by making them look like separate buttons and not drawing more background width than necessary for each item. We will also add Computing the longest width is too slow with something like 10k properties. And if you compute the width only of current matches, the background width would change as you scroll. |
Which one has larger appearance change, especially compared to regular console?
It's not nice when they have different width. As I already said no other menus look like this.
I don't see how is this complicated. You draw the selected item background over the whole background. And if you really think rounded corners are important enough here (and not just bringing it up for the sake of argument), you can draw them already, there is no need to wait for libass.
There is no problem if you make the background fixed width, like I originally mentioned. I will end my remark by pointing out that the uosc playlist selection menu uses the drawing methods I mentioned and looks much better than this PR. It additionally calculates the longest item's width, but we don't have to do that and instead keep it a constant width to avoid width calculation. |
I would prefer to not cover whole screen in background-box, just because we are too lazy to calculate the width of it. It will look obnoxious for short menu items. |
dfe0518
to
341173f
Compare
Updated as request. It disables the item shadow with background-box and draws a rectangle as wide as the longest item (the longest calculation is not perfect with unicode and proportional fonts but works in practice). The rectangle coloring is actually handled by background-box automatically. Also the background width no longer changes as you type or scroll.
I meant that we now have to draw 3 rectangles to not make the semitransparent selected rectangle overlap with the default one, but that's fine. |
bb63a9b
to
fc6be30
Compare
fc6be30
to
d537e91
Compare
It was just missing the closing ASS tag with non-nil font after I added the font. Also made it calculate the width even with outline-and-shadow so I can use it to position a scrollbar later. |
d537e91
to
b6e370f
Compare
Works. @na-na-hi: is this solution acceptable to you? |
b6e370f
to
c3c397a
Compare
Background looks OK, but the current width calculation is a little buggy and doesn't cover the whole width when it needs to. You can easily test with Also because the default font is proportional, it causes text overflow especially with Unicode.
We can copy what uosc does if we don't want to be lazy: keep a length cache for items (including regular and bold styles) so that the bounds don't need to be recomputed while selecting/scrolling. Alternatively we can always cover like 2/3 of the screen width and clip long lines. |
c3c397a
to
37b2d58
Compare
To clarify my final thought: using a default rounded corner means a change in mpv's design language, which would be a commitment to implement rounded corner changes to at least OSD bar and OSD bar is a part of the default user experience and is something in mpv's control, so that can be changed now. There will be no other suggestions from me for the rounded corner change. Feel free to ignore this and merge when it's ready otherwise if it's not considered a blocker. |
In my opinion OSD bar is not compatible with OSC. It is good to have if you use mpv without OSC, but it is already not compatible with interface. |
I'd definitely like rounded corners on the OSD bar, even already suggested it in the previous PR, but it's work that needs to be done. |
Have you evaluated yourself whether that request makes sense and whether if there are better solutions for it? I don't think blindly taking requests without evaluation is a good idea. Otherwise mpv will become even a bigger mess because everyone's ideas can go in no matter how good or bad they are.
Not before I mentioned uosc in the PR. Otherwise, the text overflow issue should be fixed already before the layout change.
As mentioned several times before it's not the longest display width. And menu width changing literally isn't a problem for now because the status quo has the same problem, so no regression. Again, I have never asked the menu to be center aligned.
It's likely that you followed that tweet just because borderless "looks nice", because you have not taken a hint from other points outlined in that tweet, all of which enhance usability and readability. Where is the different background color for search bar? Where are the extra paddings? I noticed both of these issues immediately after the new layout was first posted, long before you posted that tweet. I didn't mention it because there were other more important issues need to be addressed first.
Stop going "kasper this, kasper that". Evaluate suggestions on their own merit. There is no need for his agreement to see why that suggestion makes sense.
Those who dislike it often don't show up immediately. It has happened countless of times before that there really isn't any consensus when disagreements show up only after the changes are merged and shoved down to their throats.
Consistency/familiarity was the reason mentioned. That screenshot by itself should be apparent. |
IMO the same can be said for other OSD text because they use borders instead of a background by default, like OSC. The OSD bar also needs a redesign to fit into the OSC design theme. |
I don't decide which ideas get in, evaluation is already what PRs are for, and I don't personally care about background-box, those who use it can argue it better which is exactly what happened.
Well duh? There was no custom background drawing before you mentioned uosc, so no need for clipping, which I added along with center alignment.
It's only the status quo if one was using the non-default background-box. It looked bad to someone like me not used to it.
I followed all of his advices since 2017, they are gold.
Not being implemented yet doesn't mean I haven't considered the hint. This PR is already huge without more additions, and it is not as simple as doing it in a web page when font and background colors are configurable which can make the input line look mismatched or decrease readability.
In the
Disagreeing is an evaluation. It is normal to be skeptical until more people agree.
No idea what the point here is as there will obviously people disliking whatever we do and whether it has good reasons or not. I've seen e.g. users complaining that left clicking track buttons should show a menu which was reverted. The majority of developers' preferences is the best guess of what is better. |
78dbba7
to
afbf7b3
Compare
Add a scrollbar to give a visual representation of the size and position in the items. It is not clickable. The counter is moved above the scrollbar to free up a line for an another item, simplify calculations, and prevent the selected item from jumping from the second to the first line as you type. In terminal output there is no easy way to draw a scrollbar to the right due to Unicode widths. This just moves the counter into the prompt.
Draw a background behind selectable items even with outline-and-shadow. This makes sense for a menu because you want it to be readable, select something and move on. It doesn't stay open while watching like OSD messages and subtitles, so it can cover more of the video. In fact this was probably the only menu without a background by default. Also the scrollbar without a background looked weird, and the background shows the new horizontal hit box. --osd-outline-color determines the background color with outline-and-shadow, while --osd-back-color determines the background color with background-box. Some transparency is added because using pure black is not recommended because it causes eye strain; alternatively --osd-outline-color could default to #222222. Drawing the background ourselves also allows making the corners rounded. Free-form text mode keeps using only background-box backgrounds if configured as covering the whole screen while searching stats key bindings would be bad. Searching history also doesn't add a background to not change the layout abruptly. When searching history with background-box, it preserves the alpha component of --osd-back-color.
This reverts commit ad0c29e. This should be unnecessary now that the menu has a visible horizontal and vertical hitbox, and was not discoverable anyway. Right click can be useful even while the console is open to pause or open a context menu. Requested in mpv-player#15145 (comment).
The default item also has the same background color but with transparency. Also stop bolding selections since inverted black and white backgrounds should be visible even with color blindness. It was annoying with proportional fonts because it misalignes similar strings. As mpv's default text colors are white on black border or background, --osd-selected-color's default of a bright yellow meant to be used with a black border becomes unreadable with the inverted white background. We could default to a dark --osd-selected-color and a a light --osd-selected-outline-color and use --osd-selected-outline-color as the selected back color. However in show-text commands having only the selected item with a different white border doesn't look good. This therefore adds indipendent selected_color and selected_back_color script-opts. --osd-selected-color is only used for completions and for the selected item when searching the command history with outline-and-shadow.
It's fine to call them menus now that they actually look like ones.
afbf7b3
to
25c7305
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all the actual issues were ironed out by now.
Well I would agree the default item background could use a bit more opacity, I had it before but kasper asked to decrease it. But I think adding unique colors to it would be annoying because that's 2 extra things you have to configure when you change the colors compared to reusing |
I think that as long as the outstanding effect of the activity item can be improved, the additional customizations are no longer necessary. |
I have not experiment with it myself to dial perfect balance between current item highlight and selected item highlight. I think the current item (under mouse pointer) should be more visible, because this is what user want to chose. Selected item otoh is just an information. Both are important, but if they have the same visual impact it makes the list bloated and not readable in the sense, that user cannot differentiate between highlights. That's my opinion. Also remember that initial implementation was not readable as we can see in #15711 (review) . Hence why it was reduced to make the contrast better. I don't mind improving this, but we need to keep in mind to keep it visually pleasing and readable at the same time. |
Yeah the initial one was unreadable but we then increased the alpha further from |
This needs to strike a fine balance between being visible and having contrast with the text color and not taking attention away from the selected item. Make it slightly more opaque to differentiate it more easily. Fixes mpv-player#15711 (comment)
This needs to strike a fine balance between being visible and having contrast with the text color and not taking attention away from the selected item. Make it slightly more opaque to differentiate it more easily. Fixes mpv-player#15711.
This needs to strike a fine balance between being visible and having contrast with the text color and not taking attention away from the selected item. Make it slightly more opaque to differentiate it more easily. Fixes #15711.
console.lua: improve the hovered item calculation with background-box
Follow up to c438732 which improved the calculation with the default outline-and-shadow. We can make the calculation accurate for background-box too without making semitransparent rectangle backgrounds overlap by adding margin between items.
We could calculate the height of each item to make them perfectly adjacent by rendering each one with compute_bounds, but that takes 15ms per render, so 20 lines would take 300ms, which is too slow.
Instead add a fixed 0.1 * opts.font_size to each item's height. This ensures the backgrounds don't overlap with tall CJK text or track circles and --osd-shadow-offset=0. Add this to outline-and-shadow too since it looks better with some margin between the items. Then add the rectangle offset to the height depending on the border style.